Skip to content

1888/dense linop - #2042

Open
MarcelKoch wants to merge 9 commits into
1888/rename-densefrom
1888/dense-linop
Open

1888/dense linop#2042
MarcelKoch wants to merge 9 commits into
1888/rename-densefrom
1888/dense-linop

Conversation

@MarcelKoch

@MarcelKoch MarcelKoch commented Jul 2, 2026

Copy link
Copy Markdown
Member

This PR adds a Dense matrix format.

@MarcelKoch MarcelKoch self-assigned this Jul 2, 2026
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:example This is related to the examples. reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering type:multigrid This is related to multigrid type:stopping-criteria This is related to the stopping criteria mod:all This touches all Ginkgo modules. labels Jul 2, 2026
@MarcelKoch MarcelKoch mentioned this pull request Jul 3, 2026
9 tasks
@MarcelKoch
MarcelKoch force-pushed the 1888/renaming branch 2 times, most recently from 04359e0 to aa6583d Compare July 20, 2026 16:11
@MarcelKoch
MarcelKoch force-pushed the 1888/dense-linop branch 2 times, most recently from 6c1727e to 8728fb4 Compare July 20, 2026 16:35
@MarcelKoch
MarcelKoch changed the base branch from 1888/renaming to 1888/rename-dense August 7, 2026 13:38
@MarcelKoch
MarcelKoch changed the base branch from 1888/rename-dense to 1888/renaming August 7, 2026 13:41
@MarcelKoch
MarcelKoch changed the base branch from 1888/renaming to 1888/rename-dense August 7, 2026 13:45
@MarcelKoch MarcelKoch added this to the Ginkgo 2.0 milestone Aug 17, 2026
}

const LinOp* beta,
LinOp* x) const GKO_NOT_IMPLEMENTED;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because here is marked not implemented. the corresponding kernel and common hook can also be removed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corresponding kernel is used in matrix::Dense.

@yhmtsai yhmtsai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submit the partial review. Github seems to bring some changes in rename-dense here currently

Comment on lines 482 to 483

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two can also be removed

Comment thread core/matrix/multivector_kernels.hpp Outdated
Comment on lines 24 to 30

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed

Comment on lines -320 to -326
if (dynamic_cast<const Diagonal<ValueType>*>(b)) {
exec->run(multivector::make_add_scaled_diag(
make_temporary_conversion<ValueType>(alpha)
->get_const_device_view(),
dynamic_cast<const Diagonal<ValueType>*>(b),
this->get_device_view()));
} else {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diagonal part?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much this is needed for the vectors. It is already available for dense so it can be done via vec->as_dense_view()->add_scaled(diag). But I can also add it here (or move it from dense to here).

Comment thread core/test/matrix/multivector.cpp
Comment thread core/test/utils/matrix_generator.hpp Outdated
* @param exec executor where the matrix should be allocated
* @param args additional arguments for the matrix constructor
*
* @return the unique pointer of gko::matrix::MultiVector<ValueType>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return the unique pointer of gko::matrix::MultiVector<ValueType>
* @return the unique pointer of gko::matrix::Dense<ValueType>

if ((mode & permute_mode::rows) == permute_mode::rows) {
// compute P * A
permutation_csr->apply(input, result);
result = permutation_csr->multiply(input);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check multiply interface

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you asking here?

{
csr1 = Csr::create(exec);
csr1->copy_from(dense1);
csr1->copy_from(dense1->as_const_dense_view().get());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be created as dense not multivector?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are used as vectors everywhere but here.

Comment on lines +1472 to +1480
auto mtx = gko::share(MultiVector::create_with_config_of(
this->l_dense->as_multivector_view()));
this->l_dense->apply(this->u_dense->as_const_multivector_view(), mtx);
auto ilu_factory =
gko::preconditioner::Ilu<value_type, false, index_type>::build()
.with_l_solver(LowerIsai::build())
.with_u_solver(UpperIsai::build())
.on(this->exec);
auto ilu = ilu_factory->generate(mtx);
auto ilu = ilu_factory->generate(mtx->as_const_dense_view());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how user will use dense/multivector.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here it could also be done with storing one multivector and one dense object, but I think I was lazy.

Comment on lines -93 to +97
auto permuted = gko::as<typename TestFixture::Mtx>(
this->star_mtx->permute(&perm_array));
auto permuted = this->star_mtx->as_multivector_view()
->permute(&perm_array)
->as_dense_view()
->clone();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the clone is necessary to keep the return value of permute alive.

Comment thread reference/test/solver/ir_kernels.cpp Outdated
Comment on lines +422 to +426
using initial_guess_mode = gko::solver::initial_guess_mode;
auto ref_solver =
auto ref_factory =
gko::solver::Ir<value_type>::build()
.with_criteria(gko::stop::Iteration::build().with_max_iters(1u))
.on(this->exec)
->generate(this->mtx);
.on(this->exec);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to generate ref_solver here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't remember. Seems to work without it, so I will revert it

@MarcelKoch
MarcelKoch force-pushed the 1888/dense-linop branch 2 times, most recently from ea2b468 to 01e46b2 Compare August 18, 2026 14:57
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
- diagonal extractable
- scaled identity addable
- add/sub scaled diagonal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:ready-for-review This PR is ready for review 1:ST:skip-full-test mod:all This touches all Ginkgo modules. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:example This is related to the examples. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers type:stopping-criteria This is related to the stopping criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants